Updated test to check for copyright #803
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThis pull request updates copyright year ranges across approximately 45 configuration and source files throughout the codebase, and refactors the copyright header validation in the test suite with new helper functions for year formatting, git-based year tracking, and centralized header validation logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/cloudai/workloads/ddlb/slurm_command_gen_strategy.py`:
- Line 2: The file header's copyright year range is inconsistent with other
files; update the top comment in slurm_command_gen_strategy.py to use the
standardized range (e.g., change "Copyright (c) 2024-2026" to the agreed range
used across the repo such as "2024-2026" or "2025-2026") so all updated files
share the same copyright years; locate the header comment at the top of the file
and replace the existing year range text accordingly to match the standard used
across the project.
In `@tests/test_check_copyright_headers.py`:
- Line 47: CURRENT_YEAR is evaluated at import time which can cause failures
across a year boundary; move the computation into get_commit_years_for_file so
the year is calculated at call time. Update get_commit_years_for_file to compute
current_year = datetime.now().year (or use datetime.utcnow() if needed for
timezone consistency) and use that local variable instead of the module-level
CURRENT_YEAR constant; remove or deprecate the top-level CURRENT_YEAR to avoid
stale values.
- Around line 76-86: Add a negative test that verifies
_format_years_to_ranges([]) raises the expected ValueError: create a new test
function (e.g., test_format_years_to_ranges_empty_raises) that calls
_format_years_to_ranges with an empty list and uses pytest.raises(ValueError,
match="Expected at least one year") to assert the error and message; place it
alongside the existing tests in tests/test_check_copyright_headers.py.
- Around line 127-141: Update the assertions in _assert_copyright_in_file to
include actual vs expected values and file context to improve debuggability: for
the SPDX check (actual_copyright_lines[0]), the years check
(actual_copyright_lines[1] vs expected_years_line) and the tail check
("\n".join(actual_copyright_lines[2:]) vs HEADER_TAIL) include the file name
plus both expected and actual values in each assertion message; reference the
variables actual_copyright_lines, expected_years_line, HEADER_TAIL and
HEADER_LINES so the failure output clearly shows what was expected and what was
received.
- Around line 96-106: The git subprocess invocations that populate `res` (the
two subprocess.run calls using ["git", "log", ...] and any other git
subprocess.run calls like git status) lack return-code checks and can fail
silently; update these calls to either use check=True or inspect
`res.returncode` and throw a descriptive exception (or raise
subprocess.CalledProcessError) including `res.stderr` and the command arguments
when non-zero, so the test fails with a clear error instead of returning an
incorrect year value; ensure you update all occurrences around the
`res`/`path_str` usage (the git log/status subprocess.run calls) accordingly.
Comment on lines
+76
to
+86
| @pytest.mark.parametrize( | ||
| ("years_list", "expected"), | ||
| ( | ||
| ([2024, 2026], "2024, 2026"), | ||
| ([2024, 2025, 2027], "2024-2025, 2027"), | ||
| ([2024], "2024"), | ||
| ([2024, 2025, 2026], "2024-2026"), | ||
| ), | ||
| ) | ||
| def test_format_years_to_ranges(years_list: list[int], expected: str): | ||
| assert _format_years_to_ranges(years_list) == expected |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a test for the empty list case.
The function raises ValueError for an empty input list (lines 56-60), but there's no test case verifying this behavior. Adding a test ensures the error handling remains intact.
💡 Suggested test case
def test_format_years_to_ranges_empty_raises():
with pytest.raises(ValueError, match="Expected at least one year"):
_format_years_to_ranges([])🤖 Prompt for AI Agents
In `@tests/test_check_copyright_headers.py` around lines 76 - 86, Add a negative
test that verifies _format_years_to_ranges([]) raises the expected ValueError:
create a new test function (e.g., test_format_years_to_ranges_empty_raises) that
calls _format_years_to_ranges with an empty list and uses
pytest.raises(ValueError, match="Expected at least one year") to assert the
error and message; place it alongside the existing tests in
tests/test_check_copyright_headers.py.
Comment on lines
96
to
106
| res = subprocess.run( | ||
| ["git", "log", "--format=%ad", "--date=format:%Y", "--follow", "-1", file], | ||
| ["git", "log", "--format=%ad", "--date=format:%Y", "--follow", "--", path_str], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if not res.stdout: | ||
| # in some cases when a file was renamed, --follow won't allow getting the last modified year | ||
| if not res.stdout.strip(): | ||
| res = subprocess.run( | ||
| ["git", "log", "--format=%ad", "--date=format:%Y", "-1", file], | ||
| ["git", "log", "--format=%ad", "--date=format:%Y", "--", path_str], | ||
| capture_output=True, | ||
| text=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
Missing error handling for git subprocess calls.
The function doesn't check returncode from the subprocess calls. If git log or git status fails (e.g., not a git repo, corrupted repo, git not installed), the function may silently return incorrect results like [CURRENT_YEAR] instead of raising an informative error.
🛡️ Suggested fix to add error checking
res = subprocess.run(
["git", "log", "--format=%ad", "--date=format:%Y", "--follow", "--", path_str],
capture_output=True,
text=True,
)
+ if res.returncode != 0 and res.returncode != 128: # 128 = no commits match
+ raise RuntimeError(f"git log failed for {path_str}: {res.stderr}")
if not res.stdout.strip():
res = subprocess.run(
["git", "log", "--format=%ad", "--date=format:%Y", "--", path_str],
capture_output=True,
text=True,
)
+ if res.returncode != 0 and res.returncode != 128:
+ raise RuntimeError(f"git log failed for {path_str}: {res.stderr}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| res = subprocess.run( | |
| ["git", "log", "--format=%ad", "--date=format:%Y", "--follow", "-1", file], | |
| ["git", "log", "--format=%ad", "--date=format:%Y", "--follow", "--", path_str], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if not res.stdout: | |
| # in some cases when a file was renamed, --follow won't allow getting the last modified year | |
| if not res.stdout.strip(): | |
| res = subprocess.run( | |
| ["git", "log", "--format=%ad", "--date=format:%Y", "-1", file], | |
| ["git", "log", "--format=%ad", "--date=format:%Y", "--", path_str], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| res = subprocess.run( | |
| ["git", "log", "--format=%ad", "--date=format:%Y", "--follow", "--", path_str], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if res.returncode != 0 and res.returncode != 128: # 128 = no commits match | |
| raise RuntimeError(f"git log failed for {path_str}: {res.stderr}") | |
| if not res.stdout.strip(): | |
| res = subprocess.run( | |
| ["git", "log", "--format=%ad", "--date=format:%Y", "--", path_str], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| if res.returncode != 0 and res.returncode != 128: | |
| raise RuntimeError(f"git log failed for {path_str}: {res.stderr}") |
🤖 Prompt for AI Agents
In `@tests/test_check_copyright_headers.py` around lines 96 - 106, The git
subprocess invocations that populate `res` (the two subprocess.run calls using
["git", "log", ...] and any other git subprocess.run calls like git status) lack
return-code checks and can fail silently; update these calls to either use
check=True or inspect `res.returncode` and throw a descriptive exception (or
raise subprocess.CalledProcessError) including `res.stderr` and the command
arguments when non-zero, so the test fails with a clear error instead of
returning an incorrect year value; ensure you update all occurrences around the
`res`/`path_str` usage (the git log/status subprocess.run calls) accordingly.
podkidyshev
requested review from
amaslenn,
jeffnvidia and
srivatsankrishnan
as code owners
Contributor
Greptile OverviewGreptile SummaryThis PR updates copyright headers across 52 files to accurately reflect the years when files were modified, based on git commit history. The main improvement is in
Key improvements:
Confidence Score: 5/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fix copyright and the related check according to the rule: copyright years must cover all but only the years when the file was changed.
Test Plan
Unit-tests are enough + verified a sample of affected files manually using gitlense vscode extension that shows commit history per file.
Additional Notes
A single file my change its path in the repo yet still stay interpreted as the same
filein the history. The updated check covers this caseI have also tried to run constant number of git commands per session (O(1), not O(number of files)) but that doesn't cover the above-mentioned case.